Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure returned versionId is a string when listing objects #1193

Merged
merged 4 commits into from
Aug 10, 2023

Conversation

fflorent
Copy link
Contributor

Returned versionId is a string when calling statObject(), as it is read without transformation (casting) from the headers.

However, the xml parser casts automatically what looks like a number in the responses. Consequentially, when the S3 storage provider returns a VersionId with only numbers, listObjects() returns an object with a versionId as a number, which is inconsistent with what returns other methods like statObjects() and may result in erratic behaviors for the program using these methods.

@trim21
Copy link
Contributor

trim21 commented Jul 20, 2023

maybe #1133 ?

@fflorent
Copy link
Contributor Author

maybe #1133 ?

I don't think so, in my case versionIds are in base 10:

> const { XMLParser } = require("fast-xml-parser")
undefined
> const  fxp = new XMLParser({
...   numberParseOptions: {
...     leadingZeros: false,
...     hex: false,
...   }
... })
undefined
> fxp.parse('<Key>0x2f</Key>') // results in string as expected
{ Key: '0x2f' }
> fxp.parse('<Key>1234</Key>') // This number encoded in base 10 results in number, I would like to force them to be string for versionId
{ Key: 1234 }

@trim21
Copy link
Contributor

trim21 commented Jul 20, 2023

then I think we should create a new parser and disable auto number parsing, and parse when needed, not convert it back to string...

@fflorent
Copy link
Contributor Author

then I think we should create a new parser and disable all number, not convert it back to string...

There are some case it is relevant to convert values to numbers, like for size. Should we then convert numbers manually?

@trim21
Copy link
Contributor

trim21 commented Jul 20, 2023

Should we then convert numbers manually?

Yes, what I mean

@trim21
Copy link
Contributor

trim21 commented Jul 20, 2023

fox example, versionId like hex or 0123, versionId.toString() it not expected

@fflorent
Copy link
Contributor Author

Right, however, the only way I found to discard automatic conversion is to use the skipLike option, which is hackish:

  const parseXml = new XMLParser({
    numberParseOptions: {
      skipLike: /./
    }
  })

Documentation of numberParseOption: https://github.com/NaturalIntelligence/fast-xml-parser/blob/edb30ddefb9c131a724761b5f79cd4f883736214/docs/v4/2.XMLparseOptions.md#numberparseoptions

Related issue: NaturalIntelligence/fast-xml-parser#516

Is it fine for you?

@trim21
Copy link
Contributor

trim21 commented Jul 20, 2023

yes, looks fine

@fflorent
Copy link
Contributor Author

Done!

I did not make the same change for parseListObjectsV2 and parseListObjectsV2WithMetadata API as they don't have VersionId attribute to handle and keys / prefixes are parsed as string.

Do you think it's worth to make the same change as well despite of this?

src/xml-parsers.js Outdated Show resolved Hide resolved
Returned versionId is a string when calling statObject(), as it is read
without transformation (casting) from the headers.

However, the xml parser casts automatically what looks like a number in
the responses. Consequentially, when the S3 storage provider returns a
VersionId with only numbers, listObjects() returns an object with a
versionId as a number, which is inconsistent with what returns other
methods like statObjects() and may result in erratic behaviors for the
program using these methods.
@trim21
Copy link
Contributor

trim21 commented Aug 6, 2023

@prakashsvmx

@prakashsvmx prakashsvmx merged commit 4f53378 into minio:master Aug 10, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants